First attempt to bind constants by andriish · Pull Request #155 · RosettaCommons/binder

andriish · 2021-04-26T13:08:57Z

This is the first attempt to bind constants in binder.

The implementation is in the files const.hpp and const.cpp, which structurally are very similar to enum.hpp and enum.cpp.
The current version binds only the int/float/strings to the types described in https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html. So far this is done exclusively for those types as 1) they use python attributes 2) to make sure nothing else is affected.

Potentially this can be extended to more interesting cases of constant vectors and arrays and more generic types.

Best regards,

Andrii

lyskov

@andriish sorry for the late reply, - too much things on my plate lately! Thank you for adding this! I did quick pass and highlight some things that we will need to address before this could be merged. I will do a few more reads later. Thanks,

lyskov · 2021-05-11T18:56:06Z

source/const.cpp

+				clang::LangOptions lang_opts;
+				lang_opts.CPlusPlus = true;
+				clang::PrintingPolicy Policy(lang_opts);
+				init->printPretty(rso, NULL, Policy);


Please replace NULL with 0 here

lyskov · 2021-05-11T18:57:11Z

source/const.cpp

+			std::string type = E->getType().getCanonicalType().getAsString();
+			std::string pytype = "";
+			bool pytype_set = false;
+//This is a list of types that can bi binded with pybind, see  https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html*/


could you please fix indentation here and remove */ at the line end?

lyskov · 2021-05-11T19:00:53Z

source/const.cpp

+// Generate binding for given function: py::enum_<MyEnum>(module, "MyEnum")...
+std::string bind_const(std::string const & module, VarDecl const *E)
+{
+		string r=" ";


we need to use \t instead of just space to keep indentation correct

lyskov · 2021-05-11T19:01:46Z

source/const.hpp

+	clang::NamedDecl const * named_decl() const override { return E; };
+
+	/// check if generator can create binding
+    bool bindable() const override;


sorry, pedantic: indentation here is broken

lyskov · 2021-05-11T19:04:26Z

test/T60.const.hpp

+#include <string>
+#include <vector>
+
+const int global_int=1;


Here and below: could you please fix the const placement here and below to match the rest of source code? Thanks,

lyskov · 2021-05-11T19:10:56Z

test/T60.const.hpp

+const std::string global_string2=std::string("Some test string");
+
+
+const double expression_global_double=8.0+1.0/5.0;


I see that this will be bound verbatim below as pybind11::float_(8. + 1. / 5.) which is problematic. Consider this: let's say we binding constant for some deep-nested namespace that depend on some other constants like this:

namespace TEST { int const A = 1; int const B = A+1 ...

if binding code for this will be generate as A+1 then result will not compile because expression should really be `TEST::A +``
My recommendation would be to disable bindings for expression unless we can somehow evaluate them (this might be possible though since LLVM have required code)

Yes, that is a problematic part.

we can somehow evaluate them

Eh, it is safer not to evaluate them. Just imagine a code

#ifdef MYFLAG int const a=5; #else int const a=6; #endif

and then somewhere binding the constants like

int const i_want_to_bind_it mya = 100 * a;

In case the evaluation would be turned on, one would have to regenerate the binding code each time the 'a' is updated or the MYFLAG is changed.

What could make more sense is to transform the RHS from the "raw" code into a canonical representation. I will have to look how to do that (LLVM should be able to do that, or am I wrong?).

lyskov · 2021-05-11T19:14:12Z

test/T60.const.hpp

+
+const double expression_global_double=8.0+1.0/5.0;
+
+const double array_global_double[5]={1.0,2.0,3.0,4.0,5.0};  //This should not appear in bindings so far.


here and below: could you please add _not_binded suffix to all names thats should not be bound? That will make test script to raise alarm before compilation/linking failure which will be more readable for developers. Thanks,

lyskov · 2021-05-11T19:17:48Z

test/T60.const.hpp

+
+int foonamespaced_foo_char(char *) { return 0; }
+
+}


Can we add some user defined data types here? If they can not be bound that totally fine but i want to make sure that Binder is always generate compilable code. Thanks,

The constants can be bound using the special types like "float_", so I custom types cannot use it, as far as I understand.

lyskov · 2021-05-11T19:19:28Z

source/const.cpp

+/// extract include needed for this generator and add it to includes vector
+void ConstBinder::add_relevant_includes(IncludeSet &includes) const
+{
+}


Why this function is empty, was this just an oversight or we can not deduce includes for some reason? Can we just do binder::add_relevant_includes(E, includes, 0);?

My understanding was that once only the standard types are used, no extra includes are needed.
And the std::string is included anyway.

lyskov · 2021-05-11T19:20:37Z

source/const.cpp

+	if ( E->getType().getTypePtr()->isArrayType()) return false; 
+	if( E->isCXXInstanceMember()  or  E->isCXXClassMember() ) return false;
+	if( E->isCXXInstanceMember() ) return false;
+	if ( standard_name( E->getType().getCanonicalType().getAsString() ) == "const std::string" ) return true;
+	if (E->getType().getTypePtr()->isRealFloatingType() ) return true;


could you please fix formatting here so it consistent? Sorry: i am pedantic...

lyskov · 2024-04-12T22:08:25Z

@andriish i am reviewing old PR's that have not been active for a while. Looks like #155 (comment) might be show stopper for this one so i am incline to close this PR until new ideas emerge. Thoughts?

Andrii Verbytskyi added 3 commits April 26, 2021 14:58

First attempt to bind constants

f6ede53

Uncomments tests

e1828c7

Drop debug changes in T00

b49b5fd

lyskov requested changes May 11, 2021

View reviewed changes

Andrii Verbytskyi added 4 commits May 30, 2021 11:12

Merge remote-tracking branch 'upstream/master' into bindcostant

38b4899

Reply to review

a2d154e

More fixes to formating

cb83ff1

More fixes to formating

d2361cd

Andrii Verbytskyi added 2 commits April 13, 2024 07:38

Merge remote-tracking branch 'origin/master' into bindcostant

008d334

Sync with master

8bb5333

		const std::string global_string2=std::string("Some test string");


		const double expression_global_double=8.0+1.0/5.0;


		const double expression_global_double=8.0+1.0/5.0;

		const double array_global_double[5]={1.0,2.0,3.0,4.0,5.0}; //This should not appear in bindings so far.

Conversation

andriish commented Apr 26, 2021

Uh oh!

lyskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyskov commented Apr 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants